allergies: Property-based tests. #735
Conversation
|
From the failed build: Also, I'm sorry for the confusingly named |
There was a problem hiding this comment.
No instance for (Show Allergen)
I wished I could tell you something like "then you should ensure there is one (otherwise it's not possible for this to be merged, by repository restrictions)", but it looks to me that both the example solution and the stub do this. So, did I miss something that would explain why this is happening?
I think I would like to have these, as
- we've needed a replacement for trinary and octal for some time.
- it's quite possible to agree that the example-based tests could possibly have some gaps.
I considered asking for some material in .meta/hints.md explaining the presence of these tests, but not even octal or trinary had such hints, nor does simple-linked-list. That's a little surprising. Will have to think about how this should be introduced to students.
So how about this. If the property-based tests were the last tests in their respective describe sections, then by the time students get there, hopefully they're already equipped to pass them. Currently, they're the first test and a student encountering this sort of testing for the first time might not know what's going on.
So I'd suggest one of the two following:
- Move them to be the last tests in their respective
describe - Add an explanation, either in test comments or in
.meta/hints.md(which, upon README generation, is to be added to the README)
Is this related to the I've moved them to be the last tests in their respective Do you think I should add shrinking, @petertseng? |
|
Travis CI passes for this one now. |
There was a problem hiding this comment.
Do you think I should add shrinking
Usually to answer these questions I think about what it will look like for an implememtation that has some mistake in it.
I think it would make sense for the tests with multiple allergens. Remove any one allergen, probably.
I have a few other ideas for tests that would be useful. currently the isAllergicTo tests are testing that the implementation returns true when it should.
I think there's a lot of value gained by also checking that it returns false when it should. otherwise you might imagine that an implementation of const True would pass all the isAllergicTo tests (all right, there are existing example-based tests that would figure it out, but I am thinking it could be good to get coverage from the property-based tests too)
|
Indeed. For the Ocaml tests, I did generate negative samples. |
|
Negative properties and shrinking added. |
QuickCheck depends on `Show` instance for `Allergies`. Positive and negative properties are added. Some shrinking occurs for tests with multiple allergens.
There was a problem hiding this comment.
we're ready to go here. I wonder how many submissions this exercise gets a day and therefore how many students will come into contact with this. If students get interested we might see about having more of these. Maybe putting quickcheck as one of the topics will help mark them.
|
It doesn't get a lot. I think there has been one since Jeremy reset the non-core queue. Also, it gets unlocked after |
I was adding the
allergiesexercise to the Ocaml track and wanted to write some property-based tests for it, since that seemed quicker.I found Ocaml's QCheck library hard to understand, so I decided to write them in Haskell and port them to Ocaml. I don't know if they're really needed here, since the coverage for this exercise is already quite fine. But here they are.